Fix deduplication of -enable-experimental-feature #1044
Conversation
There was a problem hiding this comment.
Thank you for the contribution! Please see my comment asking whether we can handle this inside to_starlark(). Also, could we add https://github.com/sersoft-gmbh/swift-smtp to one of the examples so that we reproduce the issue and confirm that it stays fixed?
| @@ -106,7 +106,7 @@ def _swift_target_build_file(pkg_ctx, target): | |||
| if len(defines) > 0: | |||
| attrs["defines"] = bzl_selects.to_starlark(defines) | |||
| if len(copts) > 0: | |||
| attrs["copts"] = bzl_selects.to_starlark(copts) | |||
| attrs["copts"] = bzl_selects.unwrap_list(copts) | |||
There was a problem hiding this comment.
Is there any way to detect this in bzl_selects.to_starlark()? It is not obvious to me why copts would get this special handling.
There was a problem hiding this comment.
The reason is that we add a string repeatedly:
to_starlark relies on using sets within its processing, which will deduplicate the string that's added.
I'm not familiar with the repository, so I decided to add a new function to reduce the chances that I'll affect other places that use to_starlark.
Possible Alternatives:
- Add a parameter like
dedup = True, and only perform deduplication after the list has been created, so that ifdedup = Falsewas passed, it will return the list without deduping. - Move adding
-enable-experimental-featureafter runningto_starlark, so that it skips the deduplication. This allows us to reuseto_starlarkas it is now. Experimental features will need to be placed in a different list, so it doesn't affect the othercoptsfrom unsafe flags.
There was a problem hiding this comment.
I tried doing -enable-experimental-feature after running to_starlark, but didn't realise that it returns a structure instead of a list. So this method won't work, as to_starlark will have copts in an internal structure. (I did not dig into whether it'll be easy to append values into that list).
I'll give adding the dedup parameter a try (maybe renamed to preserve_list?)
|
@Dreksh Were you going to pick this up again? If not, I am inclined to close this. We can always open a new PR when/if you want to continue. |
|
Alternatively, we could convert it to a Draft PR. |
When using a Swift package that contains multiple experimental features (i.e. https://github.com/sersoft-gmbh/swift-smtp ), the error that is returned shows treats one of the features as an input file.
This is due to the generated parameters within the
*.swiftmodule-0.paramsfile that is used to set the build arguments, where-enable-experimental-featurewas deduplicated. The deduplication happens duringbzl_selects.to_starlarkas it operates on sets, instead of lists.Changes:
bzl_selects.unwrap_listto operate on lists instead of setsunwrap_list, as it shouldn't be treated as a set.